Update across the board ownership changes, url updates #26
Conversation
iainnash
force-pushed
the
fix_url_generation_editing_and_ownership_changes
branch
from
3cda71d to
46e4ed2
Compare
| setParams({ | ||
| [queryKeys.targets]: proposal.actions!.map((action) => action.to).join("|"), | ||
| [queryKeys.calldatas]: proposal.actions!.map((action) => action.data).join("|"), | ||
| [queryKeys.values]: proposal.actions!.map((action) => action.value).join("|"), | ||
| ...(proposal.nonce |
There was a problem hiding this comment.
great to have a nonce in here!
| )} | ||
| </SafeInformation> | ||
| </View> | ||
| <ProposalContext.Provider value={proposal}> |
There was a problem hiding this comment.
can we avoid having this in a context and instead pass it down as props? it can be tough to track where context comes from, and also context changes are mutable and dont predictably cause a state change.
There was a problem hiding this comment.
did this change in a follow on PR #27
| @@ -76,7 +74,7 @@ const AddOwnerModalContent = ({ onClose }: { onClose: () => void }) => { | |||
| }} | |||
| > | |||
| <Form> | |||
| <Text variant="featured-2">Add Owner</Text> | |||
| <Text variant="featured-2">Add Signer</Text> | |||
There was a problem hiding this comment.
Should the other variables be changed to signer too? noticed they all still are called owner
|
Do you think this issue would be alleviated if we use the context
everywhere and remove the other state you suggested? would increase perf
and speed up the component tree. another cool benefit is we could auto
update the url from set state through a useUpdateUrl effect.
…On Mon, Apr 22, 2024 at 20:10 Dan Oved ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/app/NewSafeProposal.tsx
<#26 (comment)>:
> - {isEditing && (
- <EditProposal
- proposal={proposal}
- setProposal={setProposal}
- setIsEditing={setIsEditing}
- />
- )}
- {!isEditing && proposal && (
- <ViewProposal
- proposal={proposal}
- handleEditClicked={handleEditClicked}
- />
- )}
- </SafeInformation>
- </View>
+ <ProposalContext.Provider value={proposal}>
can we avoid having this in a context and instead pass it down as props?
it can be tough to track where context comes from, and also context changes
are mutable and dont predictably cause a state change.
—
Reply to this email directly, view it on GitHub
<#26 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAGMCODLDMMXTY44YSLBUSDY6VHAPAVCNFSM6AAAAABGRAVWA2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMJVGQ4TMNZXGY>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
I tend to avoid using context unless there is a deep component hirhearchy and passing down the props many layers becomes tedious (or if its impossible) - it has downsides in that it's tricky to track state changes and it is a mutable state, which often doesn't cause components to re-render (this was happening before). Explicitly passing down props has more predictable results and makes sure the components properly re-render and the UI reflects whats in the state Since we want to be able to pass the url around, we can keep the url as the primary state/source of truth, and update that accordingly; this way the state will be reflected in the current url, and we shouldn't need to duplicate that in a context. And yeah it also makes sense to remove that other state (it's just a duplicate really)
|
…icate state, fix creation of a new proposal (#27) * Better naming around provider updating/namely: `addAction` and `replaceProposal` * Replace ProviderContext with explicit prop passing; remove duplicate `proposal` state and just have query string be source of truth * Fix some type failing issues * When there is no current proposal, and a user is to be added/removed from the safe, redirect to a `new` url so that the url can be properly shared
Fix URL changes, ownership updates.
Allow appending delete add operations for users to the current transaction queue.